Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Experimental - Task to Promise conversion #1567

Merged
merged 6 commits into from
Oct 12, 2023

Conversation

Xicy
Copy link
Contributor

@Xicy Xicy commented Jun 28, 2023

@lahma
Copy link
Collaborator

lahma commented Jun 28, 2023

Great initiative here too. I don't have strong opinions, but might open a can of worms when people would start submitting issues with corner cases - but of course they can be referred to create a PR to improve things. It seems that this would handle the most requested cases anyway.

/cc @twop

if (!task.IsCompleted)
{
// Task.Wait has the potential of inlining the task's execution on the current thread; avoid this.
((IAsyncResult) task).AsyncWaitHandle.WaitOne();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this mean that JS thread will be blocked until task is resolved?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, when promise needs to resolve, block to js thread until resolved.

I'm not sure but Evalutate function in unit tests, only process the script. It does not block until call RunAvailableContinuations

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think that is the main problem that I encountered with the Task mapping too.

Tasks (plural in general case) can be resolved out of order at any time on any thread. But in JS it is one thread that collects continuations in the order they came in, in other words non blocking at any time. So I would probably advocate to use that as a guiding principal, and to my understanding the current code is not aligned with that, or I'm missing something obv?

Copy link
Contributor Author

@Xicy Xicy Jun 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my understanding the current code is not aligned with that

As per your explanation, yes code does not align. Maybe EventLoop works on another thread that can be closer to working asynchrony.

By the way, I don't have experience with that. If you lead the way, I try to enhance the code.

Edit:
PS: When I thought about works in another thread, It may occurs an error, because the engine does not thread-safe.

@lahma
Copy link
Collaborator

lahma commented Jul 1, 2023

At the same time I see great value in this but do you folks see possible bad drawbacks (via bug reports and hard to track errors)? This would create a reasonable bridge (AFAIK) to allow common patterns and interoperability. Of course bug reports can be closed with "please send a PR to fix instead", but there's some fundamental impossibility - that might not be fair.

@VelvetToroyashi
Copy link
Contributor

VelvetToroyashi commented Aug 17, 2023

Wanted to poke at this to see where things were in terms of progress; this is great, and I'm excited to seeing these changes be implemented, however in my attempt at testing it, I've noticed a strange quirk.

Take the following C# code:

var engine = new Engine().SetValue("asyncMethod", () => Task.Delay(2000));

And in JS:

(
  async () => // Workaround for no top-level async/await
  {
    await asyncMethod();
  }
)();

When calling engine.Execute with that script, it runs as expected; the code is delayed for 2 seconds, and that's that.

However, if the delegate is instead made async, and awaits the task that represents the work:

async () => await Task.Delay(2000)

The promise is resolved immediately which is unexpected behavior, especially considering that on the surface, practically identical. My initial suspicion was it was something to do with how the task is handled, but I suspect this may be down to something deeper in the TPL implementation? I'll take a deeper look at this later in the day, but an interesting behavior nonetheless.

6 AM EDIT:

Instant-resolving issue seems to be fixed now? I don't believe I've touched anything, but now I'm concerned that I cannot reproduce this consistently; earlier, any time the delegate was asynchronous, the promise resolved instantly. No longer an issue.

Interestingly, something that remains consistent is that a Task-returning delegate (() => <Task>) returns undefined, whereas an async delegate async () => returns TaskVoidResult, though in theory both should return undefined or similar?

using Jint;

var engine = new Engine(s => s.Strict());

engine.SetValue("log", (Action<object>)Console.WriteLine);
engine.SetValue("taskDelegate", () => Task.Delay(100));
engine.SetValue("asyncDelegate", async () => await Task.Delay(100));
engine.Execute
(
    """
    (
        async () => 
        {
            let td = await taskDelegate();
            let ad = await asyncDelegate();
            log(`Task-delegate returns undefined: ${td === undefined}`);
            log(`Async-delegate returns undefined: ${ad === undefined}`);
        }
    )();
    """
);

Produces:

Task-delegate returns undefined: true
Async-delegate returns undefined: false

8/18 edit:

Turns out this is due to how the TPL works, and should be a relatively easy fix. I'll open a pull against this PR's branch momentarily.

@Xicy
Copy link
Contributor Author

Xicy commented Aug 21, 2023

@VelvetToroyashi, Thank you also here :),
Is there any chance to check failed test cases?

@VelvetToroyashi
Copy link
Contributor

@Xicy I can run the tests locally here in a second

@ejsmith
Copy link
Contributor

ejsmith commented Aug 22, 2023

This looks awesome! Can't wait to get this ability. I was wondering if we can await a C# task from the a script... would it be possible to freeze and serialize the state of the JS engine at that point and resume it at a later time? I'm thinking of long running workflow type scenarios where there is a call out from the script to wait for some event to occur. That is implemented in the form of an await inside of the script that is calling a C# async method. That async method could then serialize engine state, wait some undefined amount of time and when that event occurs it would deserialize the engine state and resume the script execution. Thoughts?

Could potentially use this to provide the long running task support:
https://github.com/Azure/durabletask

@sebastienros
Copy link
Owner

@ejsmith maybe not rely on Jint as the core but use it as a way to run an activity in the workflow. Check Elsa Workflows or the one in Orchard Core (Else is coming from there) which are using Jint to run customizable tasks.

@ejsmith
Copy link
Contributor

ejsmith commented Aug 22, 2023

@ejsmith maybe not rely on Jint as the core but use it as a way to run an activity in the workflow. Check Elsa Workflows or the one in Orchard Core (Else is coming from there) which are using Jint to run customizable tasks.

Yeah, I'm going to look into that as well. Was just thinking it would be really nice and simpler to just have a script that did the entire workflow.

@sebastienros
Copy link
Owner

@ejsmith why not after all, currently Elsa/OrchardCore are using a Json description of it. Now I am starting to understand your vision. And being able to serialize the whole state and continue is something that would be helpful in other scenarios (it has bugged us many times). Even with some constraints like assuming the global objects are immutable so they don't have to be part of the serialization. Or that we can't even store state and it's up to the workflow engine to store what it needs and rehydrate it when it's resumed (custom C# bindings, state objects, ...).

@lahma
Copy link
Collaborator

lahma commented Aug 22, 2023

I think that the easiest way to handle "game state" is to have a separate state object that can be JSON serialized. JavaScript engine state is far too complex to serialize successfully as one can mutate so many things (functions etc).

@sebastienros
Copy link
Owner

@lahma exactly, that's what I meant by "it's up to the workflow engine to store what it needs and rehydrate it"

So the remaining problem is to be able to stop the engine, maybe with a custom API call invoked from a C# callback, which would return some kind of serializable code pointer, and they another call to "resume" a script instead of "execute". I have no idea how it could work with the current implementation. Meaning that I believe that is technically impossible without changing how a script is currently processed.

@ejsmith
Copy link
Contributor

ejsmith commented Aug 22, 2023

I think that the easiest way to handle "game state" is to have a separate state object that can be JSON serialized. JavaScript engine state is far too complex to serialize successfully as one can mutate so many things (functions etc).

Yeah, I can see that. I'd think you'd have to restrict it to no global state and just serialize the call stack and variables passed to those stacks and then be able to jump back to that stack. But I'm sure that it's way more complicated than that. 🙂

@ejsmith
Copy link
Contributor

ejsmith commented Aug 22, 2023

@lahma exactly, that's what I meant by "it's up to the workflow engine to store what it needs and rehydrate it"

So the remaining problem is to be able to stop the engine, maybe with a custom API call invoked from a C# callback, which would return some kind of serializable code pointer, and they another call to "resume" a script instead of "execute". I have no idea how it could work with the current implementation. Meaning that I believe that is technically impossible without changing how a script is currently processed.

Thanks! I'm going to dive into that.

I currently have a really good scripting setup for custom logic and actions was just thinking that it would be really nice to just add something like await waitForDays(12) right into the script and various other kinds of awaits for different triggers. I think Elsa will work really well too. Just didn't want to get into having to do some sort of designer for building workflows. Does Elsa have a web designer?

@sebastienros
Copy link
Owner

Else does have a web designer.

@sfmskywalker
Copy link

@sfmskywalker
Copy link

This looks awesome! Can't wait to get this ability. I was wondering if we can await a C# task from the a script... would it be possible to freeze and serialize the state of the JS engine at that point and resume it at a later time? I'm thinking of long running workflow type scenarios where there is a call out from the script to wait for some event to occur. That is implemented in the form of an await inside of the script that is calling a C# async method. That async method could then serialize engine state, wait some undefined amount of time and when that event occurs it would deserialize the engine state and resume the script execution. Thoughts?

Could potentially use this to provide the long running task support: https://github.com/Azure/durabletask

This is super interesting to me. I've been experimenting with a custom DSL that reads like JS, but one where keywords and statements translate to actual workflow activities, making it easy to describe a long running process that reads sequentially. As an example:

variable age = GetInput("age")";

if age > 16 then
{
    print("Enjoy your driver's license! And be careful!");
    event("crash");
    print("I told you to be careful -_-");
}
else
    print("Enjoy your bicycle!");
                        
print("Goodbye.");

In the above pseudo code, if/else keywords translate to Elsa's If activity where the first body becomes a Sequence activity for the Then property and the else body becomes a Sequence activity for the Else property. print translates to the WriteLine activity,

The relevant part here is the event function, which translates to the Event activity, which is a blocking activity that puts the workflow in a suspended state, and gets resumed when the application triggers the "crash" event.

But there are two significant limitations with this DSL for the moment:

  • It's not finished
  • It doesn't support all of the scripting stuff that a real JavaScript interpreter does.

So, I think that the idea to actually use real JavaScript and have it yield to the workflow engine, and have that resume the script at some point, is brilliant. If we can somehow pull this off, I might not have to write a custom DSL in the first place to be able to define a workflow in a (limited) scripting language. Instead, an entire workflow, both long-and short-running, could be defined in JS 🤯

And although this could be supported inside a JS activity running in a workflow, I think it would make just as much sense, if not more, to also have the ability to just write .js files to define long running processes - which is the point of the custom DSL.

I have no clue what it would take to make Jint support something like this (being able to yield back to the host app mid-script and then return), but let's keep marinating on it 🤔

@twop
Copy link
Contributor

twop commented Sep 27, 2023

I think the best way to achieve that in JS without async/await is to use generators (look at return(v) method)
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/yield*#method_forwarding

A couple of libraries in JS ecosystem use that to build custom async DSL:

  1. https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/yield*#method_forwarding
  2. https://www.effect.website/docs/effect-vs-promise#comparing-effectgen-with-asyncawait

Hope that helps, note using generators + yield might be a different lift for embedders

@sfmskywalker
Copy link

sfmskywalker commented Sep 27, 2023

yield sounds like it could be a perfect fit. If I understand correctly, then it might be used like this from a RunJavaScritpt activity:

const foo = 'bar";
yield*; // Yields back to the RunJavaScript activity, which will now create a bookmark. In most samples I've seen, yield* is used to generate data (e.g. yield* 42), but in our case, we just want to yield control back. Maybe it would be nice to make that explicit, like yield* suspend or whatever.
const someInput = getInput(); // Input would be provided by the application resuming the JS bookmark.
return someInput;

@sfmskywalker
Copy link

sfmskywalker commented Sep 27, 2023

And I wonder, if that works, then could we also yield control from within a custom JS function? So that we could write a long-running workflow in pure JS:

const approveSignalUrl = generateSignalUrl("approve");
const rejectSignalUrl = generateSignalUrl("reject");

await sendEmail("approver@acme.com", `<a href="${approveSignalUrl}">Approve</a> or <a href="${rejectSignalUrl}">Reject</a>`);

if(signalReceived("approve")) // signalReceived should yield control back to engine.
{
   await sendEmail("contributor@acme.com", "Great job!");
}

if(signalReceived("reject"))
{
   await sendEmail("contributor@acme.com", "You and your work suck!");
}

In C#, signalReceived would somehow be able to tell Jint to yield, e.g.:

engine.SetValue("signalReceived", () => engine.YieldMagic("signalReceived"));

But that might be too farfetched. Perhaps the following approach might be more realistic, where the yield* keyword is returned in front of the suspending activity:

if(yield* signalReceived("approve")) // signalReceived should yield control back to engine.
{
   await sendEmail("contributor@acme.com", "Great job!");
}

if(yield* signalReceived("reject"))
{
   await sendEmail("contributor@acme.com", "You and your work suck!");
}

Here, the engine would understand that the SignalReceived activity is to be scheduled next, and when that gets resumed, it knows from the bookmark from where to resume JS execution.

@sfmskywalker
Copy link

Looking at the sample script, it would be important that the yield* keyword used in this way doesn't stop running the remaining code in order to also create a bookmark for the "reject" signal.

That got complicated real fast 😅

@ejsmith
Copy link
Contributor

ejsmith commented Sep 27, 2023

Yeah, I would personally prefer this type of long running workflow system over a visual designer. As we talked about before, the thing we'd need to make this possible is the ability for a Jint engine instance to be paused, all the callstack and state to be serialized, then later be able to come back and deserialize that callstack into a new Jint engine instance and resume from the await statement where it left off. So it would look like this:

const approveSignalUrl = generateSignalUrl("approve");
const rejectSignalUrl = generateSignalUrl("reject");

await sendEmail("approver@acme.com", `<a href="${approveSignalUrl}">Approve</a> or <a href="${rejectSignalUrl}">Reject</a>`);

// await a signal to awaken the script, tell it which signals we care about
let response = await signalRecieved("approve", "reject");
// signalRecieved is a C# async method that freezes the jint engine, serializes it
// and persists the state somewhere. C# code waits for one of those URLs to be hit
// which then triggers C# code that rehydrates the JINT engine callstack and
// resumes with a response saying which signal was triggered.

if (response.signal == "approve")
   await sendEmail("contributor@acme.com", "Great job!");
} else
   await sendEmail("contributor@acme.com", "You and your work suck!");
}

@ejsmith
Copy link
Contributor

ejsmith commented Sep 27, 2023

BTW, all of this first requires this Task to Promise conversion PR to be accepted which we have totally hi-jacked. Sorry @Xicy! @lahma what are the chances this PR is going to be accepted?

Xicy and others added 3 commits October 11, 2023 19:20
ExecutionCanceledException used for canceled operations
This commit fixes an issue where an `async Task` (be it a method or a delegate)
would end up being marshalled directly to JS, giving a `Task<VoidTaskResult>`
to the user, instead of `undefined`, which is what is returned for
"synchronous tasks", i.e. any Task-returning invokable function that does
not generate an async state machine of its own (that is to say, any function
that returns `Task`, not `async Task`.

This commit fixes the issue by checking if a Task's result is equal to
`VoidTaskResult`, which is an internal type used by the runtime to indicate
a void-returning Task, such as that from an `async Task` method/delegate
@lahma lahma force-pushed the feature/task-promise-conversion branch from 6c23400 to 5b996d5 Compare October 11, 2023 16:20
@lahma
Copy link
Collaborator

lahma commented Oct 11, 2023

@Xicy thank you for the PR, looks good and kudos for the test coverage. I've rebased against main and it still seems to have some problems running against full .NET framework, could you please take a look?

@Xicy
Copy link
Contributor Author

Xicy commented Oct 11, 2023

@Xicy thank you for the PR, looks good and kudos for the test coverage. I've rebased against main and it still seems to have some problems running against full .NET framework, could you please take a look?

First of all, I'm sorry for your loss. I'll try to take care of it next weekend.

Update:

I fixed the error, but I couldn't actually test it, because I changed my work stack dotnet to PHP and I didn't dotnet environment Could you review the PR @lahma

@lahma lahma merged commit c905f53 into sebastienros:main Oct 12, 2023
3 checks passed
@lahma
Copy link
Collaborator

lahma commented Oct 12, 2023

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants